Add support for combining system:partition and features/extras in valid_systems#3613
Conversation
…id_systems, as requested in reframe-hpc#2820
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3613 +/- ##
========================================
Coverage 91.37% 91.37%
========================================
Files 62 62
Lines 13484 13484
========================================
Hits 12321 12321
Misses 1163 1163 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I've done some concrete testing to prove this works. Note that I have 4 partitions (rome, genoa, gpu_A100, gpu_H100), of which the first two have the **Case 1: traditional `system:partition` assignment**Output: **Case 2: Combining system:partition with +feat1**Output: **Case 3: Combining system:partition with -feat1**Output: **Case 4: Using only features**This case clearly doesn't make any sense, because it'll create tests where the first element of Output: |
|
Hm, I have to recheck how the current |
|
|
||
| _S = rf'({_NW}(:{_NW})?)' # system/partition | ||
| _VALID_SYS_SYNTAX = rf'^({_S}|{_FKV}(\s+{_FKV})*)$' | ||
| _VALID_SYS_SYNTAX = rf'^(({_FKV}\s+)*{_S}(\s+{_FKV})*|{_FKV}(\s+{_FKV})*)$' |
There was a problem hiding this comment.
The PR would benefit from a rewrite of this line.
The system definition should not rely on spaces.
All of these definitions should be valid
1. *
2. daint:gpu
3. daint:mc
4. daint:mc +gpu
5. daint:mc +gpu +uenv
6. daint:mc+gpu
7. daint:mc+gpu +uenv
8. daint:mc +gpu+uenv
9. daint:mc +gpu +uenv+modules
10. daint:mc +gpu+uenv +modules
In the current form only the first four cases are valid.
The fifth case is miss interpreted, where one cannot see the +gpu entry.
And the last five case are not valid systems.
This implies that:
- Tests won't run because of a
space, without any hint. Users may lose a lot of time because of a missingspace. - Potential introduction of silent bugs. The addition of a new feature makes the previous invalid.
- Hinders the ability to write tests with partitions that have multiple features.
We should make this feature more robust to the end user and to the future.
What do you think?
There was a problem hiding this comment.
The system definition should not rely on spaces.
Just to be clear: unless I'm mistaken, the current version of ReFrame also requires spaces between features, right?
All of these definitions should be valid
And same here: definitions 6-10 are not valid in the current reframe either?
Don't get me wrong: I'm not against allowing for non-space separated features. But I'm also not sure it should be handled in this PR: allowing non-space seperated features is orthogonal to being able to combine the sysname:partname syntax with the +feat syntax. And I wouldn't want this PR to be delayed because of discussions on whether or not non-space separated features are desirable.
The fifth case is miss interpreted, where one cannot see the +gpu entry.
That's strange, so you're saying that if you define:
daint:mc +gpu +uenv
You're effectively getting
daint:mc +uenv
?
I haven't observed that, but I might have not tested this particular case. I'll try it out...
There was a problem hiding this comment.
I cannot reproduce this. I have two partitions, A and B. A has feature foo and B has features foo and bar. If I create a test and set
self.valid_systems = ['sysname:* +bar +foo'
your issue was that bar would be ignored. However, I tested this, and it (correctly) only schedules a test for partition B, which offers bar.
|
@casparvl I see the problem you are facing, but I don't agree with the solution conceptually. Allowing a syntax like The problem is more fundamental. It is the fact that This pattern is not unique to |
|
@vkarak Sorry for the slow response, I was on holiday.
Hm, coming from the more traditional syntax before features/extras where a thing, I see your point. But your solution makes the pretty hard assumption that the In this context, having both a I'm not saying there aren't any other options: if we have access to configuration objects in the after-init hook (as discussed in #3323 ), I guess we could take the But: such an approach does feel very clunky, as we're bypassing the whole feature mechanism. Compared to that, I'd find the |
to be blunt, i don't see what's contradictory here. i understand why it looks strange to you, but this PR solves our problem in a relatively simple and generic way. is there any technical reason why this can't be merged? |
4 participants
This was requested in #2820 , although that was eventually closed with a suggestion for a workaround.
However, I hit it again when trying to use
reframe.utils.find_modules(substr, ...)in combination with wanting to specify additional features. Example use cases (and there's many more one can come up with):CPUfeature to my CPU partitions (but not to my GPU partitions) and then requesting theCPUfeature from my CPU-only tests.infinibandfeature.In both cases, we want to test all modules with
XorYrespectively that are available on our system, hence we want to use thefind_modulesfunction. However, the first element of the tuple this returns is thesystem:partitionstring for which it found the module matching the requested substring. This is clearly meant to be used invalid_systemsdirectly (as is confirmed by the example in the docs), but for the examples above, you may want to only run on a subset of thesesystem:partitioncombinations (based on the required features. It would be very natural to then do:To make sure that a partition is only valid if it provides the right module and the right feature.
It turned out that the code changes needed are very limited (it seems like a lot because the indentation changed, but the key part is:
elifon the subspec:And adding that
syspart_matchto the return logic:Note that this logic does not assume the
system:partitionto strictly come before or after the features/extras, so I've adapted the valid syntax specification to allow both:If anything, I feel the code has become cleaner by not handling
system:partitionas a special, separate case, but as 'just another item' that may occur as a subspec.